Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fleet RBAC - InheritedFleetWorkspacePermissions validation #348

Merged
merged 16 commits into from
Jun 24, 2024
Merged

Fleet RBAC - InheritedFleetWorkspacePermissions validation #348

merged 16 commits into from
Jun 24, 2024

Conversation

raulcabello
Copy link
Contributor

Issue: rancher/rancher#44901

Problem

Need to add validation for the new fields in GlobalRole InheritedFleetWorkspacePermissions

Solution

  • Validate the user have enough permission to create/update the rules defined in InheritedFleetWorkspacePermissions.ResourceRules
  • Validate the user have enough permission to create/update the rules that are generated based on the InheritedFleetWorkspacePermissions.WorkspaceVerbs
  • Don't allow to modify the labels authz.management.cattle.io/gr-owner and authz.management.cattle.io/grb-owner in ClusterRole and ClusterRoleBinding

CheckList

  • Test
  • Docs

@raulcabello raulcabello requested a review from a team as a code owner April 5, 2024 15:16
@raulcabello raulcabello mentioned this pull request Apr 5, 2024
Merged
MbolotSuse
MbolotSuse suggested changes Apr 8, 2024
View reviewed changes
Copy link
Contributor

@MbolotSuse MbolotSuse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of items:

  • You need escalation checks for GlobalRoleBindings as well as GlobalRoles
  • Please update the docs for the GlobalRole checks.
  • The escalation logic is a bit flawed. The approach that we use needs to consider these permissions on their own since it gives permissions on all current workspaces and all future workspaces (besides local).

pkg/resources/management.cattle.io/v3/globalrole/validator.go Show resolved Hide resolved
pkg/resources/rbac.authorization.k8s.io/v1/clusterrole/validator_test.go Show resolved Hide resolved
pkg/resources/rbac.authorization.k8s.io/v1/clusterrolebinding/validator_test.go Show resolved Hide resolved
pkg/resources/management.cattle.io/v3/globalrole/validator.go Outdated Show resolved Hide resolved
@raulcabello raulcabello force-pushed the inherited-fleet-roles branch 3 times, most recently from cf97831 to 006c813 Compare April 11, 2024 08:18
@raulcabello raulcabello requested a review from MbolotSuse April 11, 2024 08:49
MbolotSuse
MbolotSuse suggested changes Apr 15, 2024
View reviewed changes
Copy link
Contributor

@MbolotSuse MbolotSuse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor issues and one overall larger issue regarding escalation checking.

pkg/auth/globalrole.go Outdated Show resolved Hide resolved
pkg/resources/management.cattle.io/v3/globalrole/validator.go Outdated Show resolved Hide resolved
pkg/resources/rbac.authorization.k8s.io/v1/clusterrole/validator.go Outdated Show resolved Hide resolved
pkg/resources/rbac.authorization.k8s.io/v1/clusterrolebinding/validator.go Outdated Show resolved Hide resolved
@raulcabello raulcabello requested a review from MbolotSuse April 19, 2024 14:12
MbolotSuse
MbolotSuse suggested changes Apr 22, 2024
View reviewed changes
Copy link
Contributor

@MbolotSuse MbolotSuse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few concerns on the architecture/code structure, but the functionality/escalation checking looks much better.

pkg/auth/globalrole.go Outdated Show resolved Hide resolved
pkg/auth/globalrole.go Outdated
return nil
}

var rules []rbacv1.PolicyRule
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than allocating a new slice and copying the rules over, it might be good to just return what is in the role - like gr.InheritedFleetWorkspacePermissions.ResourceRules

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

pkg/resolvers/grbClusterResolver.go Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The core object here is called a GRBClusterRuleResolver. I don't think that we can make this evaluate other types of rules (i.e. permissions on/in the workspace) without a name change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed to GRBRuleResolver

pkg/resolvers/grbClusterResolver.go Outdated Show resolved Hide resolved
pkg/resolvers/grbClusterResolver.go Outdated Show resolved Hide resolved
pkg/auth/globalrole.go Show resolved Hide resolved
pkg/auth/globalrole.go Show resolved Hide resolved
pkg/resources/management.cattle.io/v3/globalrole/validator.go Outdated Show resolved Hide resolved
pkg/resources/management.cattle.io/v3/globalrolebinding/validator.go Outdated Show resolved Hide resolved
pkg/resources/rbac.authorization.k8s.io/v1/clusterrolebinding/validator_test.go Outdated Show resolved Hide resolved
@raulcabello raulcabello force-pushed the inherited-fleet-roles branch from 36016dc to 9f1fb01 Compare April 23, 2024 13:35
@raulcabello raulcabello requested a review from MbolotSuse April 29, 2024 11:29
JonCrowther
JonCrowther previously approved these changes May 2, 2024
View reviewed changes
JonCrowther
JonCrowther previously approved these changes May 23, 2024
View reviewed changes
@samjustus
Copy link

#42170

@raulcabello raulcabello requested a review from JonCrowther June 14, 2024 14:04
@raulcabello raulcabello requested a review from JonCrowther June 14, 2024 15:07
JonCrowther
JonCrowther previously approved these changes Jun 14, 2024
View reviewed changes
@raulcabello raulcabello force-pushed the inherited-fleet-roles branch from 57b1a9f to 1d1092f Compare June 18, 2024 10:55
@raulcabello raulcabello force-pushed the inherited-fleet-roles branch from 1d1092f to c7b77c9 Compare June 19, 2024 08:16
@raulcabello raulcabello requested a review from JonCrowther June 19, 2024 08:16
JonCrowther
JonCrowther previously approved these changes Jun 20, 2024
View reviewed changes
MbolotSuse
MbolotSuse reviewed Jun 20, 2024
View reviewed changes
Copy link
Contributor

@MbolotSuse MbolotSuse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One NIT, but outside of that LGTM (After fixing merge conflict).

go.mod Outdated Show resolved Hide resolved
@raulcabello
InheritedFleetWorkspacePermissions validation
f690386 
- Validate the user have enough permission to create/update the rules defined in InheritedFleetWorkspacePermissions.ResourceRules
- Validate the user have enough permission to create/pdate the rules that are generated based on the InheritedFleetWorkspacePermissions.WorkspaceVerbs
raulcabello and others added 15 commits June 21, 2024 09:46
@raulcabello
Add missing generated file
2a1fd71
@raulcabello
Fix golangci-lint
377cc01
@raulcabello
Move fleetworkspace validation to GlobalRoleResolver
d08ae0d
@raulcabello
validate fleetworkspace rules in its own resolver
f10e67f
@raulcabello
validate workspaceVerbs is not empty
dbb090d
@raulcabello
improve unit test
6ffd38c
@raulcabello @MbolotSuse
remove unnecessary comment
2499660 
Co-authored-by: Michael Bolot <michael.bolot@suse.com>
@raulcabello
don't create intermediary slice
1cfd47e
@raulcabello
aggregate rule resolvers in GRBRuleResolvers
2cbaf6c
@raulcabello
remove duplicated unit test
6a6b8de
@raulcabello
Add comment in FleetWorkspacePermissionsWorkspaceVerbsFromRole
91e6554
@raulcabello
adapt code to FleetWorkspacePermissions change to be a pointer
5d11abb
@raulcabello
fix breaking unit test
08254bb
@raulcabello
Allow Restricted Admin to create GR and GRB with FleetWorkspacePermis…
44f5d22 
…sions
@raulcabello
Add missing comment to FleetWorkspacePermissionsResourceRulesFromRole
70b603a
@raulcabello raulcabello force-pushed the inherited-fleet-roles branch from c7b77c9 to 70b603a Compare June 21, 2024 07:47
@raulcabello raulcabello merged commit af2d8bd into rancher:release/v0.5 Jun 24, 2024
1 check passed
@raulcabello raulcabello deleted the inherited-fleet-roles branch June 24, 2024 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MbolotSuse MbolotSuse MbolotSuse approved these changes

@JonCrowther JonCrowther JonCrowther approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@raulcabello @samjustus @JonCrowther @MbolotSuse